Create new validation API #129

lcobucci · 2016-10-24T00:32:49Z

Yes @Ocramius @henriquemoody @schnittstabil @dannydorfel, after a looong time we finally have something to solve the validation API 🎉

Brace yourself because this is huge PR but after merging it we'll haz:

interfaces for the things that the library exposes
fix for the SRP violation
private claim validation (custom constraints)

lcobucci · 2016-10-24T00:33:05Z

There're things that can still be improved (especially naming) and still need to update the documentation, but would be amazing to have your comments!

Ocramius · 2016-10-24T08:44:49Z

src/Parser.php

-     * @return array
-     *
-     * @throws InvalidArgumentException When an invalid header is informed
+     * @throws InvalidArgumentException


Can we use an exception from this package?

Planned for #128

Ocramius · 2016-10-24T08:45:40Z

src/Storage/Builder.php

+    public function __construct(Parsing\Encoder $encoder)
+    {
+        $this->encoder = $encoder;
+        $this->headers = ['typ'=> 'JWT', 'alg' => 'none'];


Move this to defaults

What you mean? Move to the property definition?

Ocramius · 2016-10-24T08:45:45Z

src/Storage/Builder.php

+    {
+        $this->encoder = $encoder;
+        $this->headers = ['typ'=> 'JWT', 'alg' => 'none'];
+        $this->claims = [];


Move this to defaults

Ocramius · 2016-10-24T08:47:16Z

src/Storage/Builder.php

+     */
+    public function canOnlyBeUsedBy(string $audience, bool $addHeader = false): BuilderInterface
+    {
+        $audiences = $this->claims['aud'] ?? [];


Is it possible to use private constants for things like 'aud' and such? I know they are in the spec, but the verbosity is annoyingly low...

v4 is meant for php 7.0...
But the constant idea is interesting... RegisteredClaims::AUDIENCE sounds good...

However I think we should add it on a different PR

Issue please?

Ocramius · 2016-10-24T08:47:49Z

src/Storage/Builder.php

+
+        return $this->setRegisteredClaim(
+            'aud',
+            array_values(array_map('strval', $audiences)),


Should there be an array_unique() here?

It could be added indeed (although 100% unrelated with the changeset AHAHAHH)

New issue then?

Actually I'll solve it here, that array_map() + array_values() is useless now... the type hint already forces it to be string.

(and it was hurting my eyes HAHAHA)

Ocramius · 2016-10-24T09:20:28Z

src/Validation/InvalidTokenException.php

+ *
+ * @since 4.0.0
+ */
+final class InvalidTokenException extends Exception


extends InvalidArgumentException? extends UnexpectedValueException?

Exception is Lcobucci\JWT\Exception... an initial work of #128.
Didn't want to come to a big inheritance tree now.

Ocramius · 2016-10-24T09:21:06Z

src/Validation/InvalidTokenException.php

+    public static function fromViolations(ConstraintViolationException ...$violations): self
+    {
+        $exception = new self('The token violates some mandatory constraints');
+        $exception->violations = $violations;


Move the violation messages also into the message?

Ocramius · 2016-10-24T09:22:38Z

src/Validation/Validator.php

+ *
+ * @since 4.0.0
+ */
+final class Validator implements \Lcobucci\JWT\Validator


More than a validator, this looks like a composite assertion. A validator simply returns true/false, this thing is actually throwing a ConstraintViolationException containing multiple previous ConstraintViolationException instances.

It's throwing an InvalidTokenException with multiple ConstraintViolationException instances.

What would be the best name in your opinion?

AggregateAssertion or MultiAssertion works too... It would still throw a subtype of ConstraintViolationException

InvalidTokenException is not a ConstraintViolationException and my idea is that users should only know about InvalidTokenException.

Of course we could turn these into a composite specification (which is closer to what you're describing IMO) but I don't think we need that complexity...

A validator simply returns true/false

I'm a bit skeptical about it... not saying that's not true but it sounds over simplified.
Maybe we can offer both things:

interface Validator { /** * @throws InvalidTokenException */ public function assert(Token $token, Constraint ...$constraints): void; public function validate(Token $token, Constraint ...$constraints): bool; }

One would use assert() when is looking for a "detailed report" or validate() when is just caring about the end result.

In my opinion, validate should return an array of ConstraintViolation (not ConstraintViolationException!), thus assert may look like:

public function assert(Token $token, Constraint ...$constraints) { if ($violations = $this->validate($token, ...$constraints)) { throw new InvalidTokenException($violations); } }

I don't think that the validate() method name indicates that it should return a boolean. However it's not in accordance with the rest of the changes since you have assert() methods throwing exceptions while here you have a validation() method doing the same.

With that in mind I would either call this class *Assertion or change the implementation of validate() method. But, throwing an exception every time a token is validated doesn't look very nice, IMO. You may or may not want details about the validation.

Split it into two methods may be a good idea but also looks too much responsibility for the Validator. I still keep my ide of having a Result object with the violations as the result of the validate() method, which in the end will tell you if the validation is okay or not, something like:

class Result { private $violations; public function __construct(array $violations) { $this->violations = $violations } public function isValid(): bool { return empty($violations); } public function getViolations(): array { return $this->violations; } }

@henriquemoody what I dislike about having a result object is requiring users to do this when they just care about the result:

if (!$validator->validate($token, ...$constraints)->isValid()) { // or assign a variable for the result ofc }

Split it into two methods may be a good idea but also looks too much responsibility for the Validator.

Both methods are related to the same thing, so the reason to change is the same.

@dannydorfel @henriquemoody @schnittstabil @Ocramius thanks for your suggestions regarding this. I think the best option for now is splitting in two methods.

$validator = new Validator(); $validator->validate($token, ...$constraints)->isValid()

Eek, 3 times valid on a single line. Wait – 3 times?

The Validator class is final and doesn't have any state, thus it is only a container or the namespace for the validate and assert functions, isn't it? For consumers, this is very inconvenient, I think they should use those functions directly:

use function Lcobucci\JWT\assert; use function Lcobucci\JWT\validate; assert($token, ...$constraints); $result = validate($token, ...$constraints); // whatever result will be

Btw, because of this, I don't know if the Validator class and the Validator interface are need at all. Interop? – Questionable. Of course, they can be kept for internal usage, but I think providing only the functions is much more appropriate.

Another suggestion:

use function Lcobucci\JWT\assert; // clearly throws Exceptions use function Lcobucci\JWT\isValid; // clearly returns boolean values use function Lcobucci\JWT\validate; // returns ValidationResult objects, IMO acceptable

Ocramius · 2016-10-24T09:22:55Z

src/Validation/Validator.php

+            $violations = $this->assert($violations, $constraint, $token);
+        }
+
+        if (!empty($violations)) {


if ($violations) ;-)

Ocramius · 2016-10-24T09:23:17Z

src/Validation/Validator.php

+        $violations = [];
+
+        foreach ($constraints as $constraint) {
+            $violations = $this->assert($violations, $constraint, $token);


This is overwriting the previous $violations. Some confusion around this code.

Didn't want to use &$violations there...

dannydorfel · 2016-10-25T07:34:49Z

Nice! And now I understand the "case of the Mondays" tweet.
I have got nothing to add 😁

schnittstabil

ConstraintViolation instead of ConstraintViolationException – Looks like github falsely belives the comments below are outdated, please click on Show outdated manually.

I don't think we need a ConstraintViolationException class. I've seen this in many assertion/validation libraries, but just because it's sometimes convenient doesn't mean it's the right thing.

Exceptions implement getFile, getLine and getTrace, but these don't describe the location/path (e.g. nbf) within the token, it's the location within the jwt source code – rather useless for a consumer.
Exceptions are thrown, this means a ConstraintViolationExceptions describes single violation (wo composite pattern or similar) - see my ValidAt::assert comment

Instead we need a ConstraintViolation class, which fits our needs, e.g. with a specialized __toString().

schnittstabil · 2016-10-26T13:49:42Z

src/Validation/Validator.php

+ *
+ * @since 4.0.0
+ */
+final class Validator implements \Lcobucci\JWT\Validator


In my opinion, validate should return an array of ConstraintViolation (not ConstraintViolationException!), thus assert may look like:

public function assert(Token $token, Constraint ...$constraints) { if ($violations = $this->validate($token, ...$constraints)) { throw new InvalidTokenException($violations); } }

schnittstabil · 2016-10-26T14:03:47Z

src/Validation/Constraint/ValidAt.php

+        $this->assertExpiration($token);
+        $this->assertBeforeThanNow($claimSet, 'nbf', 'The token cannot be used yet');
+        $this->assertBeforeThanNow($claimSet, 'iat', 'The token was issued in the future');
+    }


The first ConstraintValidationException wins – In my opinion, we should instead implement validate:

/** * @return ConstraintValidation[] */ public function validate(Token $token) { $claimSet = $token->claims(); return array_merge( $this->validateExpiration($token), $this->validateBeforeThanNow($claimSet, 'nbf', 'The token cannot be used yet'), $this->validateBeforeThanNow($claimSet, 'iat', 'The token was issued in the future') ); } /** * @return ConstraintValidation[] */ private function validateExpiration(Token $token) …

This also means, we can drop the ugly try-catch at Validator::assert.

schnittstabil · 2016-10-26T16:13:10Z

src/Validator.php

+ *
+ * @since 4.0.0
+ */
+interface Validator


Sorry, I don't know if this was already discussed, but how do we want to use Validators?
Currently(?):

$config = new Configuration(); $validator = $config->getValidator(); $validator->validate($token, new ValidAt(new DateTimeImmutable())); $validator->validate($token, new IssuedBy('test.com'));

As Configuration serves like a small DI container, there are also other possibilities, e.g.:

// during bootstrapping $jwtConfig = new Configuration(); $jwtConfig->getValidator() ->addConstraint(new ValidAt(new DateTimeImmutable())) ->addConstraint(new IssuedBy('test.com')); // somewhere else $jwtConfig->getValidator()->validate($token);

Sorry, I don't know if this was already discussed, but how do we want to use Validators?

$config = new Configuration(); $validator = $config->getValidator(); $validator->validate( $token, new ValidAt(new DateTimeImmutable()), new IssuedBy('test.com') ); // or simply $constraints = [new ValidAt(new DateTimeImmutable()), new IssuedBy('test.com')]; $validator->validate($token, ...$constraints);

As Configuration serves like a small DI container, there are also other possibilities, e.g.:

I don't see changing the state of the service as an option, IMO that's not a good practice. Also, new ValidAt(new DateTimeImmutable()) might lead to some issues due to the creation and the validation time.

That was some food for thought – changing the state is one of many possibilities, but you're right the splat operator offers great options for code reuse. The more I think about it, the more I love it.

henriquemoody · 2016-10-29T10:53:54Z

src/Builder.php

-
-        return $this;
-    }
+    public function with(string $name, $value): Builder;


Only with() looks a little bit vague to me, wouldn't be better to call it withClaim instead?

Indeed, change needed

Those names were defined on #113 we're just extracting an interface. Let's define on #150 and change them on a different PR.

henriquemoody · 2016-10-29T11:01:59Z

src/Storage/Builder.php

+        $this->with($name, $value);
+
+        if ($addHeader) {
+            $this->headers[$name] = $this->claims[$name];


Although they're private properties and you know what with() does, I would create some method to get the claims by name, that you could also use on canOnlyBeUsedBy(), and I would also use withHeader() instead of setting $this->headers manually.

henriquemoody · 2016-10-29T11:05:07Z

src/Storage/Builder.php

+        return $this;
+    }
+
+    private function encode(array $items): string


Isn't this useful on the Encoder itself?

Not sure... sometimes you don't want to encode with JSON+Base64Url (binary data is just base64url for example)

henriquemoody · 2016-10-29T11:26:46Z

src/Validation/InvalidTokenException.php

+
+    public function violations(): array
+    {
+        return $this->violations;


This is going to break if someone use the exception with the constructor. Just to be more defensive I would either define it as an array when declaring the property or pass the violations in the constructor.

Solved, thanks 😉

malukenho · 2016-11-08T17:52:01Z

src/Builder.php

-            $addHeader
-        );
-    }
+    public function canOnlyBeUsedBy(string $audience, bool $addHeader = false): Builder;


belongsTo?

Not really, audience is about who can consume the token.
I wouldn't change it.

Docblock specifies "appends", method name suggests "only". :confus:

Those names were defined on #113 we're just extracting an interface. Let's define on #150 and change them on a different PR.

@Ocramius

Code changed... @Ocramius are you fine with merging this as is now and implement the exception improvements in another PR?

Ocramius · 2017-01-07T14:56:57Z

src/Builder.php

-            $addHeader
-        );
-    }
+    public function canOnlyBeUsedBy(string $audience, bool $addHeader = false): Builder;


Docblock specifies "appends", method name suggests "only". :confus:

Ocramius · 2017-01-07T14:57:43Z

src/Builder.php

-
-        return $this;
-    }
+    public function with(string $name, $value): Builder;


Indeed, change needed

Ocramius · 2017-01-07T14:58:47Z

src/Builder.php

-     * Returns the resultant token
-     *
-     * @return Token
+     * Returns an unsecured token (not recommended)


Wouldn't it make more sense to use the previous API with a no-op signer?

What you mean here?

Have always a token, let a NoneSignerdeal with it (validates only empty signature, generates only empty signature)

That's a good idea, it would simplify things indeed.

But it can be done in a different PR... I really want to have this merged instead of adding things to the stack

#152 is there for that.

Ocramius · 2017-01-07T15:00:18Z

src/Configuration.php

+        return $this->validator;
+    }
+
+    public function setValidator(Validator $validator)


Should have a @return void docblock

Wouldn't bother with that now since I want to merge it and work on #146

Ocramius · 2017-01-07T15:03:06Z

src/Token.php

-     * @return mixed
-     *
-     * @throws OutOfBoundsException
+     * Returns if the token minimum time is before than given time


Minimum time of what?

It validates the "not before" claim, so in theory it's the minimum time of usage but I didn't have a better name... any suggestion?

Ah, that's confusing because of the not in not before :-\

I guess that's what the spec is - can't fix the naming then.

Ocramius · 2017-01-07T15:13:14Z

src/Token/Plain.php

+     */
+    public function __toString(): string
+    {
+        return implode('.', get_object_vars($this));


Consider getting rid of @get_object_vars() here, as no order is guaranteed

Ocramius · 2017-01-07T15:15:31Z

src/Validation/Constraint.php

+     *
+     * @throws ConstraintViolationException
+     */
+    public function assert(Token $token);


Ocramius · 2017-01-07T15:16:25Z

src/Validation/Constraint.php

+    /**
+     * @param Token $token
+     *
+     * @throws ConstraintViolationException


Add @return void

Wouldn't bother with that now since I want to merge it and work on #146

Ocramius · 2017-01-07T15:21:31Z

src/Validation/InvalidTokenException.php

+            $violations
+        );
+
+        $message = 'The token violates some mandatory constraints, details:' . PHP_EOL;


I'd just use \n

Ocramius · 2017-01-07T15:22:01Z

src/Validation/InvalidTokenException.php

+        );
+
+        $message = 'The token violates some mandatory constraints, details:' . PHP_EOL;
+        $message .= implode(PHP_EOL, $violations);


Samee: \n

Simplifying the whole thing and making it easier to create new signers.

Yes this is a big change... brace yourself! The ideia here is to make things more extensible and decoupled. So what I did was: - Extract interfaces for the important stuff to isolate the namespaces - Create the DataSet class (definitely not best name) to remove duplication on headers and claims manipulation - Make (almost) everything final - Group token related stuff under a namespace - Create methods to encapsulate token verifications Fixes: #117

And so remove all the the existing mutations.

Fixing the SRP violation and adding an extensible way to validate tokens. Fixes #29 and #72.

And remove things that are not useful now that we have scalar type hints. Fixes #131

Things changed 😉

…tor-extraction"

lcobucci added the Improvement label Oct 24, 2016

lcobucci assigned Ocramius Oct 24, 2016

Ocramius previously requested changes Oct 24, 2016

View reviewed changes

schnittstabil reviewed Oct 26, 2016

View reviewed changes

lcobucci force-pushed the refactor/validator-extraction branch from 525f3dc to 2373e57 Compare October 28, 2016 12:35

henriquemoody reviewed Oct 29, 2016

View reviewed changes

lcobucci mentioned this pull request Oct 31, 2016

The library silently fails when signing a token in case of openssl error #132

Closed

lcobucci force-pushed the refactor/validator-extraction branch 3 times, most recently from 71df5ee to 5876bf4 Compare November 7, 2016 22:27

This was referenced Nov 7, 2016

Provide a high level API to simplify usage #140

Closed

Use a time provider to get the current system time #139

Closed

malukenho reviewed Nov 8, 2016

View reviewed changes

lcobucci force-pushed the refactor/validator-extraction branch 2 times, most recently from 33a81ec to 87c7fda Compare November 16, 2016 23:29

lcobucci mentioned this pull request Dec 9, 2016

Error when adding secret #148

Closed

lcobucci force-pushed the refactor/validator-extraction branch from 87c7fda to d848c69 Compare January 7, 2017 11:44

lcobucci changed the title ~~[WIP] Create new validation API~~ Create new validation API Jan 7, 2017

lcobucci assigned lcobucci and unassigned Ocramius Jan 7, 2017

Ocramius previously requested changes Jan 7, 2017

View reviewed changes

This was referenced Jan 7, 2017

Improve Builder interface methods #150

Closed

Out of nbf or exp Time Message? #151

Closed

lcobucci force-pushed the refactor/validator-extraction branch from d848c69 to 42ab523 Compare January 7, 2017 16:44

lcobucci added 11 commits January 7, 2017 17:01

Add test to multiple audience

75fbfee

Remove signature dependency from signers

b3d246f

Simplifying the whole thing and making it easier to create new signers.

Remove premature optimization and make Signer lighter

d90447e

Remove old validation api

5dd0976

Make signers final

bc5eea2

Create unit test for RSA signer base class

bdc15ce

And so remove all the the existing mutations.

Create new validation API

3d8d343

Fixing the SRP violation and adding an extensible way to validate tokens. Fixes #29 and #72.

Add validation constraints for registered claims

1ef2a86

Create the signature when retrieving the token

3ad8661

Prevent the creation of tokens with duplicated audiences

4fb4d79

And remove things that are not useful now that we have scalar type hints. Fixes #131

lcobucci force-pushed the refactor/validator-extraction branch from 42ab523 to 4fb4d79 Compare January 7, 2017 17:02

lcobucci merged commit cfe0888 into master Jan 7, 2017

lcobucci deleted the refactor/validator-extraction branch January 7, 2017 17:13

lcobucci mentioned this pull request Jan 8, 2017

Token Validation API #72

Closed

patrickunic added a commit to patrickunic/jwt that referenced this pull request Jan 10, 2017

Revert "Merge pull request lcobucci#129 from lcobucci/refactor/valida…

072816f

…tor-extraction"

lcobucci mentioned this pull request Jan 10, 2017

ValidationData class is not present in master branch #154

Closed

lcobucci added this to the 4.0.0 milestone Mar 16, 2017

Create new validation API #129

Create new validation API #129

Conversation

lcobucci commented Oct 24, 2016 • edited Loading

lcobucci commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

schnittstabil Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannydorfel commented Oct 25, 2016

schnittstabil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius Jan 7, 2017 • edited by lcobucci Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci commented Oct 24, 2016 •

edited

Loading

lcobucci Oct 26, 2016 •

edited

Loading

lcobucci Nov 1, 2016 •

edited

Loading

schnittstabil Nov 3, 2016 •

edited

Loading

schnittstabil left a comment •

edited

Loading

Ocramius Jan 7, 2017 •

edited by lcobucci

Loading